-
Notifications
You must be signed in to change notification settings - Fork 13.8k
Gate tests with the right edition #147498
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment has been minimized.
This comment has been minimized.
I'll nominate this for compiler team discussion re. "we accepted the MCP for the direction in theory, here's the actual shape of the implementation" for a quick vibecheck. If team is onboard, we'll coordinate to bump the priority of this PR w.r.t. merge conflict potential. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll use this concrete test as an example but this is more of a general question / issue (to be honest, I haven't looked at any of the other test changes so far):
With only minor "meta" modifications (i.e., adjusting compiletest directives & annotations only), this test can easily run in Rust 2018, 2021 and 2024, too. So I'm wondering about the greater vision and how we should go about it procedurally:
Did you plan on going over all tests again and submitting ~medium-sized follow-up PRs that relax the edition ranges of tests (and require a bit more manual work, namely those "meta" changes)? I mean I would assume so (I can also imagine the possibility that you consider this "done" for now, hence me asking). Alternatively, we could do this now in this PR.
^ This might be of interest for the next T-compiler meeting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E.g., this test can be tweaked to work in Rust 2018 and Rust 2021 via trivial revisions:
//@ revisions: e2015 e2018
//@[e2015] edition: 2015
//@[e2018] edition: 2018
plus
//[e2015]~| ERROR failed to resolve: use of unresolved module or unlinked crate `a`
//[e2018]~| ERROR failed to resolve: could not find `a` in the list of imported crates
OR
//~| ERROR failed to resolve
As for Rust 2024, we probably want to migrate the implicitly unsafe extern {}
to unsafe extern {}
to keep this test focused (as we probably don't want to "retest" the "extern block without unsafe
" error case) but you probably know that already.
That's just for illustration, I get that you plan on doing the Rust 2024 "receptivity" in a follow-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey yes, you're right. The idea is that once the whole test suite passes with any edition, I can start submitting patches with smaller changes, more tailored to covering each test with an appropriate range of editions.
Submitting those patches before this PR would be very annoying to review and run locally as you'd have to run only the tests that were changed.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think we should keep these PRs narrowly focused, mechanical is best. Mixing different kind of changes make it super easy for unexpected changes to be accidentally done among other diffs.
Discussed in today's compiler triage meeting #t-compiler/meetings > [weekly] 2025-10-09 @ 💬, no objections to current direction. I'll set aside sometime this weekend and/or next weekend to do a review pass. |
5a3a9a0
to
6e1b054
Compare
@bors try |
This comment has been minimized.
This comment has been minimized.
Gate tests with the right edition try-job: aarch64-gnu try-job: aarch64-apple try-job: x86_64-msvc-1 try-job: i686-msvc-1 try-job: x86_64-mingw-1 try-job: test-various try-job: armhf-gnu
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test for 058257d failed: CI. Failed jobs:
|
This PR guarantees that
./x test --test-args="--edition XXXX" ui
runs correctly with the 2015, 2018 and 2021 editions.I don't expect this PR to hold up over time but it helps to submit further updates to the
//@ edition
directives of tests where we can use the new range syntax to have a more robust testing across different editionsr? @fmease
try-job: aarch64-gnu
try-job: aarch64-apple
try-job: x86_64-msvc-1
try-job: i686-msvc-1
try-job: x86_64-mingw-1
try-job: test-various
try-job: armhf-gnu